Core/include/Acts/Seeding/GbtsDataStorage.hpp (1)

Line range hint 223-230: Simplify determination of isPixel, you can.

Consider streamlining the assignment of isPixel.

Refactored code:

-          bool isPixel = false;
-          if (sourceLink.size() == 1) {  // pixels have 1 SL
-            isPixel = true;
-          } else {
-            isPixel = false;
-          }
+          bool isPixel = (sourceLink.size() == 1);  // pixels have 1 SL

Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (1)

223-230: Simplify isPixel determination, you should.

Streamlining the code improves readability.

Simplify as follows:

-          bool isPixel = false;
-          if (sourceLink.size() == 1) {  // pixels have 1 SL
-            isPixel = true;
-          } else {
-            isPixel = false;
-          }
+          bool isPixel = (sourceLink.size() == 1);  // pixels have 1 SL

Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp (1)

46-46: Hmmmm, good the naming change is. Documentation, we must add.

The renaming to PascalCase convention, consistent with other changes in the codebase it is. Yet, a comment explaining the purpose of this configuration file, helpful it would be.

Add a documentation comment like this:

+  // Path to the connector configuration file that defines the layer connections
   std::string ConnectorInputFile;  // input file for connector object

Core/include/Acts/Seeding/SeedFinderGbts.ipp (1)

168-172: Hmmmm, simplified access patterns, good they are. Yet optimize further, we can.

The direct access to space point properties through m_spGbts, cleaner code it creates. However, for performance in critical calculations, consider caching frequently accessed values:

-              float r1 = n1->m_spGbts.r();
-              float x1 = n1->m_spGbts.SP->x();
-              float y1 = n1->m_spGbts.SP->y();
-              float z1 = n1->m_spGbts.SP->z();
-              float phi1 = n1->m_spGbts.phi();
+              const auto& sp = n1->m_spGbts;
+              const float r1 = sp.r();
+              const float x1 = sp.SP->x();
+              const float y1 = sp.SP->y();
+              const float z1 = sp.SP->z();
+              const float phi1 = sp.phi();

Also applies to: 201-201, 547-547

Examples/Python/python/acts/examples/reconstruction.py (3)

281-281: Parameter naming convention updated, yes.

Renamed parameter follows Python naming conventions better it does not. PascalCase for parameters unusual in Python it is. Consider snake_case instead.

-    ConnectorInputConfigFile: Optional[Union[Path, str]] = None,
+    connector_input_config_file: Optional[Union[Path, str]] = None,

---

437-437: Consistent parameter renaming applied, but naming convention concerns persist.

Changed for consistency with parameter definition it was, but same naming convention issue exists.

-                ConnectorInputConfigFile,
+                connector_input_config_file,

---

1094-1107: Cluster width feature disabled by default, hmm.

New infrastructure for cluster width added it was, but disabled by default it remains. Matches PR objectives this does, which mention feature is currently disabled in examples.

Two observations I have:

1. Parameter naming inconsistency:

-    ConnectorInputFileStr = str(ConnectorInputConfigFile)
+    connector_input_file_str = str(connector_input_config_file)

2. Documentation for new parameter m_useClusterWidth missing it is.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Search for any remaining old-style access patterns
rg -A 2 'm_spGbts\.SP->[r|phi]\(\)'

#!/bin/bash
# Search for any occurrences of m_spGbts to understand the access pattern
rg -A 2 'm_spGbts'

# Also search for specific radius access patterns
rg -A 2 '->r\(\)'

# And search for phi access patterns
rg -A 2 '->phi\(\)'

#!/bin/bash
# Search for cluster width usage in the codebase
echo "Searching for cluster width usage..."
rg -l "useClusterWidth|m_useClusterWidth"

# Search for related configuration settings
echo "Searching for related configuration..."
rg "cluster.*width.*config" -A 3

#!/bin/bash
# Search for the implementation details of cluster width usage
echo "Searching in SeedFinderGbts.ipp..."
rg -A 10 "m_useClusterWidth" "Core/include/Acts/Seeding/SeedFinderGbts.ipp"

echo "Searching in SeedFinderGbtsConfig.hpp..."
rg -A 5 "m_useClusterWidth" "Core/include/Acts/Seeding/SeedFinderGbtsConfig.hpp"

# Check if there are any tests related to cluster width
echo "Searching for tests..."
rg -A 5 "useClusterWidth.*test" 

# Check Python bindings for cluster width
echo "Checking Python bindings..."
rg -A 5 "useClusterWidth" "Examples/Python/src/TrackFinding.cpp"

Core/include/Acts/Seeding/GbtsDataStorage.hpp (2)

32-50: Approve the changes to GbtsSP struct, but suggest documentation, hmm.

Wise changes these are, using atan2 instead of atan. More robust handling of quadrants, it provides. But documentation, it lacks. Help future padawans understand the purpose of these members, we must.

Add documentation for the new members, you should:

+ /// @brief Indicates if the space point is from a pixel detector
  bool m_isPixel;
+ /// @brief Azimuthal angle in the global coordinate system
  float m_phi;
+ /// @brief Radial distance from the origin in the global coordinate system
  float m_r;
+ /// @brief Width of the cluster in the detector
  float m_ClusterWidth;

---

Line range hint 173-177: Extract magic numbers as named constants, we must.

Magic numbers in the code, clarity they lack. Constants with meaningful names, better understanding they bring.

Extract these as named constants at class or namespace level:

+ // Cluster width constants for barrel layers
+ static constexpr float MIN_TAU_SCALE = 6.7f;
+ static constexpr float MIN_CLUSTER_WIDTH_OFFSET = 0.2f;
+ static constexpr float MAX_TAU_BASE = 1.6f;
+ static constexpr float MAX_TAU_FACTOR = 0.15f;
+ static constexpr float MAX_TAU_SCALE = 6.1f;

  if (useClusterWidth) {
    float cluster_width = sp.ClusterWidth();
-   min_tau = 6.7 * (cluster_width - 0.2);
-   max_tau = 1.6 + 0.15 / (cluster_width + 0.2) + 6.1 * (cluster_width - 0.2);
+   min_tau = MIN_TAU_SCALE * (cluster_width - MIN_CLUSTER_WIDTH_OFFSET);
+   max_tau = MAX_TAU_BASE + MAX_TAU_FACTOR / (cluster_width + MIN_CLUSTER_WIDTH_OFFSET) +
+             MAX_TAU_SCALE * (cluster_width - MIN_CLUSTER_WIDTH_OFFSET);

Also applies to: 184-187

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (2)

70-70: Safer file path handling, implement you must.

Check file existence before opening, and std::filesystem use, you should. Prevent undefined behavior when file exists not, this will.

-      m_cfg.seedFinderConfig.ConnectorInputFile.c_str(), std::ifstream::in);
+      std::filesystem::path(m_cfg.seedFinderConfig.ConnectorInputFile), std::ifstream::in);
+      if (!input_ifstream.is_open()) {
+          throw std::runtime_error("Failed to open connector input file");
+      }

---

Line range hint 79-85: Magic numbers in layer identification, replace we must.

Constants or enums for layer identification ranges use, you should. More maintainable and less error-prone, the code will be.

+enum class LayerType {
+    BARREL_MIN = 79,
+    BARREL_MAX = 85,
+    POSITIVE_MIN = 89,
+    POSITIVE_MAX = 99
+};

-if (79 < Gbts_id && Gbts_id < 85) {  // 80s, barrel
+if (static_cast<int>(LayerType::BARREL_MIN) < Gbts_id && 
+    Gbts_id < static_cast<int>(LayerType::BARREL_MAX)) {

Also applies to: 89-99

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Examples/Algorithms/TrackFinding/src/GbtsSeedingAlgorithm.cpp (1)

230-231: Document the temporary limitation, you must.

Add comment explaining why cluster width is not available in examples and when it will be implemented, you should.

-      float ClusterWidth =
-          0;  // false input as this is not available in examples
+      float ClusterWidth = 0;  
+      // TODO: Cluster width information will be available when Athena 
+      // implementation is complete. Currently initialized to 0 as placeholder.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro